Skip to content

[clang] fix redecl chain assumption when checking linkage consistency #153996

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 19, 2025

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Aug 17, 2025

In C++, it can be assumed the same linkage will be computed for all redeclarations of an entity, and we have assertions to check this.

However, the linkage for a declaration can be requested in the middle of deserealization, and at this point the redecl chain is not well formed, as computation of the most recent declaration is deferred.

This patch makes that assertion work even in such conditions.

This fixes a regression introduced in #147835, which was never released, so there are no release notes for this.

Fixes #153933

@mizvekov mizvekov self-assigned this Aug 17, 2025
@mizvekov mizvekov changed the title [clang] fix redecl chain assumption when checking liknkage consistency [clang] fix redecl chain assumption when checking linkage consistency Aug 17, 2025
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go.

In C++, it can be assumed the same linkage will be computed for all
redeclarations of an entity, and we have assertions to check this.

However, the linkage for a declaration can be requested in the middle
of deserealization, and at this point the redecl chain is not well
formed, as computation of the most recent declaration is deferred.

This patch makes that assertion work even in such conditions.

This fixes a regression introduced in #152845, which was never released,
so there are no release notes for this.

Fixes 153933
@mizvekov mizvekov force-pushed the users/mizvekov/GH153933 branch from 0e6bb8a to 0ff4fcb Compare August 19, 2025 16:55
@mizvekov mizvekov marked this pull request as ready for review August 19, 2025 16:56
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Aug 19, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 19, 2025

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

In C++, it can be assumed the same linkage will be computed for all redeclarations of an entity, and we have assertions to check this.

However, the linkage for a declaration can be requested in the middle of deserealization, and at this point the redecl chain is not well formed, as computation of the most recent declaration is deferred.

This patch makes that assertion work even in such conditions.

This fixes a regression introduced in #147835, which was never released, so there are no release notes for this.

Fixes #153933


Full diff: https://github.com/llvm/llvm-project/pull/153996.diff

2 Files Affected:

  • (modified) clang/lib/AST/Decl.cpp (+10-7)
  • (added) clang/test/Modules/GH153933.cpp (+23)
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 12fe5516883eb..4507f415ce606 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -1604,17 +1604,20 @@ LinkageInfo LinkageComputer::getLVForDecl(const NamedDecl *D,
   // We have just computed the linkage for this decl. By induction we know
   // that all other computed linkages match, check that the one we just
   // computed also does.
-  NamedDecl *Old = nullptr;
-  for (auto *I : D->redecls()) {
-    auto *T = cast<NamedDecl>(I);
-    if (T == D)
+  // We can't assume the redecl chain is well formed at this point,
+  // so keep track of already visited declarations.
+  for (llvm::SmallPtrSet<const Decl *, 4> AlreadyVisited{D}; /**/; /**/) {
+    D = cast<NamedDecl>(const_cast<NamedDecl *>(D)->getNextRedeclarationImpl());
+    if (!AlreadyVisited.insert(D).second)
+      break;
+    if (D->isInvalidDecl())
       continue;
-    if (!T->isInvalidDecl() && T->hasCachedLinkage()) {
-      Old = T;
+    if (auto OldLinkage = D->getCachedLinkage();
+        OldLinkage != Linkage::Invalid) {
+      assert(LV.getLinkage() == OldLinkage);
       break;
     }
   }
-  assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage());
 #endif
 
   return LV;
diff --git a/clang/test/Modules/GH153933.cpp b/clang/test/Modules/GH153933.cpp
new file mode 100644
index 0000000000000..41184c6b00607
--- /dev/null
+++ b/clang/test/Modules/GH153933.cpp
@@ -0,0 +1,23 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 %t/B.cppm -emit-module-interface -o %t/B.pcm
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -fprebuilt-module-path=%t %t/C.cpp
+
+//--- A.hpp
+template<class> struct A {};
+template<class T> struct B {
+  virtual A<T> v() { return {}; }
+};
+B<void> x;
+
+//--- B.cppm
+module;
+#include "A.hpp"
+export module B;
+using ::x;
+
+//--- C.cpp
+#include "A.hpp"
+import B;

@mizvekov
Copy link
Contributor Author

The windows cxx17-compat test failure is current breakage on main, other PRs are suffering from the same failure.

@mizvekov mizvekov merged commit 5485c70 into main Aug 19, 2025
13 of 14 checks passed
@mizvekov mizvekov deleted the users/mizvekov/GH153933 branch August 19, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang][modules] Virtual function with dependent return type included and imported causes compiler to hang
3 participants